Truncate a hostname longer than 64 characters for self-signed certificates - #6900
Conversation
Motivation:
`TlsKeyPair.ofSelfSigned()` uses `SystemInfo.hostname()` as the certificate CN. On a machine whose
hostname exceeds 64 characters — e.g. a GitHub Actions macOS runner named
`sjc20-cw714-0e535ccf-aeaf-489f-8a27-1ab749ed681c-fe6ab157f2e4.local` (67 characters) — certificate
generation fails with `IllegalArgumentException: commonName length 67 exceeds RFC 5280
ub-common-name (64)`, so every test that calls the no-arg overload fails before it starts.
This broke `:athenz:shadedTest` on a macOS CI runner (`RoleTokenClientTest` and
`AthenzTokenClientErrorHandlingTest` failed at class initialization) and can hit any test that
uses the no-arg overload whenever such a runner is assigned.
Modifications:
- Replace all 20 no-arg `TlsKeyPair.ofSelfSigned()` call sites in tests with
`TlsKeyPair.ofSelfSigned("localhost")`. None of these tests depend on the certificate CN.
Result:
- Tests no longer derive the self-signed certificate CN from the machine hostname, so they pass
regardless of the hostname of the machine they run on.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughSelf-signed certificate generation now truncates hostnames longer than 64 characters when creating the subject common name. Documentation and tests cover certificate generation and server TLS setup with overlong hostnames. ChangesTLS certificate hostname handling
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…ed() Motivation: `TlsKeyPair.ofSelfSigned()` uses the local hostname as the certificate CN as is, so it fails with `IllegalArgumentException: commonName length 67 exceeds RFC 5280 ub-common-name (64)` on a machine whose hostname exceeds 64 characters, such as a GitHub Actions macOS runner. Modifications: - Truncate the local hostname to 64 characters in `TlsKeyPair.ofSelfSigned()` before generating a self-signed certificate, and document the behavior. Result: - `TlsKeyPair.ofSelfSigned()` no longer fails on a machine whose hostname exceeds 64 characters.
ikhoon
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
core/src/test/java/com/linecorp/armeria/common/TlsKeyPairTest.java (1)
27-36: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the truncation test deterministic.
This test uses the test runner's actual hostname. It exercises the new truncation branch only when that hostname exceeds 64 characters. A regression that removes truncation can pass on ordinary runners.
Extract the truncation calculation into a package-private helper, or inject the hostname source, and test it with a fixed 65-character ASCII hostname. Keep this test for the local-hostname integration path.
As per PR objectives, this test is intended to cover hostname truncation behavior.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/src/test/java/com/linecorp/armeria/common/TlsKeyPairTest.java` around lines 27 - 36, Extract the hostname truncation logic used by TlsKeyPair.ofSelfSigned into a package-private helper, then add a deterministic unit assertion using a fixed 65-character ASCII hostname that verifies the result is limited to 64 characters. Retain selfSignedWithLocalHostname() to cover the actual local-hostname integration path, but do not rely on it to exercise truncation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@core/src/test/java/com/linecorp/armeria/common/TlsKeyPairTest.java`:
- Around line 27-36: Extract the hostname truncation logic used by
TlsKeyPair.ofSelfSigned into a package-private helper, then add a deterministic
unit assertion using a fixed 65-character ASCII hostname that verifies the
result is limited to 64 characters. Retain selfSignedWithLocalHostname() to
cover the actual local-hostname integration path, but do not rely on it to
exercise truncation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3cf42b4e-79b3-4698-91ef-3ee13325fd9c
📒 Files selected for processing (2)
core/src/main/java/com/linecorp/armeria/common/TlsKeyPair.javacore/src/test/java/com/linecorp/armeria/common/TlsKeyPairTest.java
…t-hostname # Conflicts: # core/src/main/java/com/linecorp/armeria/common/TlsKeyPair.java # core/src/test/java/com/linecorp/armeria/common/TlsKeyPairTest.java
Motivation: Without a log, a truncated certificate CN is hard to trace back to the hostname length limit. Modifications: - Log a debug message with the original and truncated hostname when `TlsKeyPair.ofSelfSigned()` truncates the local hostname. Result: - Easier to diagnose a certificate whose CN differs from the local hostname.
…sts" This reverts commit f75df0a.
ikhoon
changed the title
|
Tick the box to add this pull request to the merge queue (same as
|
Motivation: macOS CI runners kept failing through `ServerBuilder.tlsSelfSigned()`, which generates a self-signed certificate via `VirtualHostBuilder` and bypasses `TlsKeyPair.ofSelfSigned()`. Modifications: - Move the 64-character truncation from `TlsKeyPair.ofSelfSigned()` to the `SelfSignedCertificate` constructor so that every self-signed certificate generation path is covered. - Add regression tests with a synthetic 67-character fqdn. Result: - Self-signed certificate generation no longer fails on a machine whose hostname exceeds 64 characters, regardless of the API used.
ikhoon
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/src/main/java/com/linecorp/armeria/common/TlsKeyPair.java (1)
141-143: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocument truncation for the explicit-hostname overload.
SelfSignedCertificatenow applies the 64-character rule to every constructor. Therefore,TlsKeyPair.ofSelfSigned(String)also truncates a longhostname. The new note covers onlyofSelfSigned()with the local hostname. Add the same behavior note to the explicit-hostname Javadoc.As per path instructions, public API behavior must be documented and the project requires Javadoc for public APIs.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/src/main/java/com/linecorp/armeria/common/TlsKeyPair.java` around lines 141 - 143, Update the Javadoc for the explicit-hostname TlsKeyPair.ofSelfSigned(String) overload to document that hostnames longer than 64 characters are truncated to satisfy the RFC 5280 common name limit, matching the existing note for the local-hostname overload.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@core/src/main/java/com/linecorp/armeria/internal/common/util/SelfSignedCertificate.java`:
- Around line 249-250: Update truncateToCommonNameLength to validate fqdn with
the existing meaningful validation utility and the parameter name "fqdn" before
calling fqdn.length(), ensuring null input produces the standard validation
exception and message.
- Around line 245-246: Update SelfSignedCertificate’s CertificateParams
construction and SignedCertificate.generate() flow so the original fqdn is
retained for automatic DNS SAN generation while only the common name is
truncated to its permitted length. Ensure the generated certificate contains the
full requested FQDN in its DNS SAN, and add coverage for an over-64-character
FQDN.
---
Outside diff comments:
In `@core/src/main/java/com/linecorp/armeria/common/TlsKeyPair.java`:
- Around line 141-143: Update the Javadoc for the explicit-hostname
TlsKeyPair.ofSelfSigned(String) overload to document that hostnames longer than
64 characters are truncated to satisfy the RFC 5280 common name limit, matching
the existing note for the local-hostname overload.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a6c8dab6-4e92-4afd-b4fa-b8dc44df5ce0
📒 Files selected for processing (4)
core/src/main/java/com/linecorp/armeria/common/TlsKeyPair.javacore/src/main/java/com/linecorp/armeria/internal/common/util/SelfSignedCertificate.javacore/src/test/java/com/linecorp/armeria/internal/common/util/SelfSignedCertificateTest.javacore/src/test/java/com/linecorp/armeria/server/VirtualHostBuilderTest.java
| super(new CertificateParams(truncateToCommonNameLength(fqdn), random, bits, notBefore, notAfter, | ||
| algorithm, subjectAlternativeNames, isCA)); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
root=core/src/main/java/com/linecorp/armeria/internal/common/util
fd -0 -t f -e java '^(CertificateParams|SignedCertificate)\.java$' "$root" |
xargs -0 -r rg -n -C 8 \
'params\.fqdn\(\)|ownerName\(\)|subjectAlternativeName|dNSName|new GeneralName'Repository: line/armeria
Length of output: 25781
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd -0 -t f -e java '^(SelfSignedCertificate|SelfSignedCertificateTest)\.java$' \
core/src/main/java/com/linecorp/armeria/internal/common/util/ \
core/src/test/java/com/linecorp/armeria/internal/common/util/ |
sort -z | xargs -0 -r wc -l
fd -0 -t f -e java '^(SelfSignedCertificate|SelfSignedCertificateTest)\.java$' \
core/src/main/java/com/linecorp/armeria/internal/common/util/ \
core/src/test/java/com/linecorp/armeria/internal/common/util/ |
sort -z | xargs -0 -r sed -n '1,380p'
# Extract SelfSignedCertificate constructor context around the truncated fqdn line.
root=core/src/main/java/com/linecorp/armeria/internal/common/util
sed -n '220,260p' "$root/SelfSignedCertificate.java" | cat -n
# Programmatically inspect whether OwnerName is truncated on SelfSignedCertificate contruction.
# This uses the source text as data; it does not compile or execute repository code.
python3 - <<'PY'
from pathlib import Path
p = Path('core/src/main/java/com/linecorp/armeria/internal/common/util/SelfSignedCertificate.java')
src = p.read_text()
start = src.index('public SelfSignedCertificate(String fqdn')
end = src.index('}', start) + 2
body = src[start:end]
print("--- SelfSignedCertificate constructor body ---")
print(body)
print("passes truncated fqdn to super:", 'new CertificateParams(truncateToCommonNameLength(fqdn)' in body)
print("passes original fqdn to super:", 'new CertificateParams(fqdn' in body)
PYRepository: line/armeria
Length of output: 18420
Preserve the original FQDN in the DNS SAN.
truncationToCommonNameLength(fqdn) is passed to CertificateParams, and SignedCertificate.generate() uses params.fqdn() as the automatic DNS SAN. A FQDN over 64 characters therefore produces a SAN that is also truncated, so hostname validation against the requested FQDN can fail. Store the original FQDN separately or pass it to SAN generation, and add a test that asserts the full FQDN in the SAN when the common name is truncated.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@core/src/main/java/com/linecorp/armeria/internal/common/util/SelfSignedCertificate.java`
around lines 245 - 246, Update SelfSignedCertificate’s CertificateParams
construction and SignedCertificate.generate() flow so the original fqdn is
retained for automatic DNS SAN generation while only the common name is
truncated to its permitted length. Ensure the generated certificate contains the
full requested FQDN in its DNS SAN, and add coverage for an over-64-character
FQDN.
| private static String truncateToCommonNameLength(String fqdn) { | ||
| if (fqdn.length() <= MAX_COMMON_NAME_LENGTH) { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Validate fqdn before measuring its length.
truncateToCommonNameLength(fqdn) calls fqdn.length() without validation. A null FQDN produces an NPE without a useful message. Validate fqdn with "fqdn" before this call.
As per path instructions, validation must use meaningful validation and exception messages.
Proposed fix
private static String truncateToCommonNameLength(String fqdn) {
+ requireNonNull(fqdn, "fqdn");
if (fqdn.length() <= MAX_COMMON_NAME_LENGTH) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private static String truncateToCommonNameLength(String fqdn) { | |
| if (fqdn.length() <= MAX_COMMON_NAME_LENGTH) { | |
| private static String truncateToCommonNameLength(String fqdn) { | |
| requireNonNull(fqdn, "fqdn"); | |
| if (fqdn.length() <= MAX_COMMON_NAME_LENGTH) { |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@core/src/main/java/com/linecorp/armeria/internal/common/util/SelfSignedCertificate.java`
around lines 249 - 250, Update truncateToCommonNameLength to validate fqdn with
the existing meaningful validation utility and the parameter name "fqdn" before
calling fqdn.length(), ensuring null input produces the standard validation
exception and message.
Source: Path instructions
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6900 +/- ##
============================================
+ Coverage 74.46% 75.16% +0.70%
- Complexity 22234 25510 +3276
============================================
Files 1963 2267 +304
Lines 82437 94558 +12121
Branches 10764 12377 +1613
============================================
+ Hits 61385 71078 +9693
- Misses 15918 17609 +1691
- Partials 5134 5871 +737 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Motivation: The hostname tag of `armeria.server.tls.handshakes` is derived from the server certificate, whose common name is now truncated to 64 characters. The test compared it against the full default hostname, so it failed on a machine whose hostname exceeds 64 characters. Modifications: - Compare the hostname tag against the certificate hostname, truncated to 64 characters. Result: - `ServerTlsHandshakeMetricsTest` passes on a machine whose hostname exceeds 64 characters.
3 participants
Motivation:
Self-signed certificate generation uses the local hostname as the certificate CN. On a machine whose hostname exceeds
64 characters — e.g. a GitHub Actions macOS runner named
sjc20-cw714-0e535ccf-aeaf-489f-8a27-1ab749ed681c-fe6ab157f2e4.local(67 characters) — it fails with:
Both
TlsKeyPair.ofSelfSigned()andServerBuilder.tlsSelfSigned()are affected, so any test using them fails whenever sucha runner is assigned (example failure:
:athenz:shadedTestfailed at class initialization).Modifications:
SelfSignedCertificateconstructor, which covers every self-signedcertificate generation path, and log a debug message when truncation happens.
Result: